…emove/list/status/run/secrets) (#602)

* feat(cli): add ado-aw disable

Squash-merge of #591 (feat/cli-disable). See original PR for review history and detailed rationale.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(cli): add ado-aw remove

Squash-merge of #592 (feat/cli-remove).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(cli): add ado-aw list

Squash-merge of #594 (feat/cli-list).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(cli): add ado-aw status

Implements PR 7 of the Phase 1 CLI overhaul. Renders per-pipeline
status — name, id, folder, queueStatus, latest-run summary, and a
deep link — one block per matched ADO definition. Read-only.

`status` is intentionally a thin renderer over the same data path as
`list` (same `list_definitions` + `match_definitions` +
`get_latest_build` sequence). The `--json` shape is byte-for-byte
identical to `list --json` so scripts can use either.

CLI surface:

    ado-aw status [PATH] --org --project --pat --json

The block renderer is a pure function (`render_blocks`) tested
against six scenarios:

- empty → placeholder line
- succeeded run with url → all fields rendered
- run with no url → synthesizes
  `{org_url}/{project}/_build/results?buildId={id}`
- no last run → "never" + no url line
- in-progress run (no `result` yet) → shows `status` value instead
- missing queueStatus → renders `?` placeholder

Depends on PR 5 (#594) for `crate::list::{ListRow, LastRun,
build_rows, render_json}`; reuses the `get_latest_build` ado/mod.rs
helper landed there.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): avoid duplicate ADO definition fetch in list and status

Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/c7c2866a-891d-48c3-8336-774bba086817

Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>

* feat(cli): add ado-aw run

Squash-merge of #595 (feat/cli-run).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(cli): rename configure to secrets with subcommands

Squash-merge of #600 (feat/cli-secrets).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): percent-encode ctx.project in all ADO URL formatters

Code-review finding on PR #602: seven URL formatters in
`src/ado/mod.rs` interpolated `ctx.project` directly into the path
segment of the request URL while five sibling functions correctly
ran the value through `percent_encoding::utf8_percent_encode(...,
PATH_SEGMENT)`. Project names containing reserved characters
(spaces, `/`, `?`, `#`, `:` etc.) would have broken the URL or
silently produced surprising responses.

Affected functions, all now using the same encoder as
`get_repository_id`, `get_definition_full`, `patch_queue_status`,
`delete_definition`, and `create_definition`:

- `list_definitions`
- `get_definition_name`
- `update_pipeline_variable` (both GET and PUT URLs)
- `queue_build`
- `get_build`
- `get_latest_build`

The `info!()` log line at the top of `match_definitions` is
unaffected (logging, not URL construction).

The existing `path_segment_*` tests already cover the encoder
behaviour; no new test is needed since these are mechanical
substitutions of an existing pattern. Full `cargo test` (1567 unit
tests + integration crates) and `cargo clippy --all-targets
--all-features` are green after the fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(run): re-check --timeout between each in-flight get_build call

Code-review follow-up on PR #602: `poll_until_complete` only checked
`started.elapsed() >= timeout` at the top of each round, so with N
in-flight builds and reqwest's 30s per-call HTTP timeout, the
operator-visible wait could overshoot `--timeout` by up to N × 30s
in the pathological all-stalled case.

Re-checks the wall-clock budget between each individual `get_build`
call inside a round. When the budget is exhausted mid-round, the
current target and every remaining one are carried forward into
`pending` so the caller's `in_progress` count stays accurate (the
loop owes a status for everything it queued).

In the common case where the poll interval is several times the HTTP
timeout, the previous behaviour was indistinguishable from the new
one — the bug only matters when poll interval ≪ HTTP timeout, which
is an awkward but plausible configuration.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): percent-encode project in secrets/run; make --value-stdin conflict explicit

Three follow-ups on PR #602:

1. `src/secrets.rs::put_definition` was the last URL formatter using
   `ctx.project` unencoded. Now uses `PATH_SEGMENT` like every other
   builder in `src/ado/mod.rs`. `PATH_SEGMENT` was promoted from
   private `const` to `pub const` to support cross-module reuse.

2. `src/run.rs` was printing a deep-link to the queued build using
   unencoded `ado_ctx.project`. The URL is cosmetic (never used as an
   HTTP target), but it would render broken/unclickable for projects
   containing spaces or other URL-unsafe characters. Now encoded with
   the same `PATH_SEGMENT` encoder.

3. `ado-aw secrets set <value> --value-stdin` silently ignored
   `--value-stdin` when both were supplied (explicit positional value
   won). Added `conflicts_with = "value"` to the `value_stdin` clap
   arg so the combination is rejected at parse time with a clear
   error. Added an integration test in
   `tests/secrets_integration.rs` to pin the behaviour.

`cargo test` (1567 unit + 14 integration crates, all green) and
`cargo clippy --all-targets --all-features` pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): encode synthesized URL, surface detect errors, document parse_parameters edge

Four follow-ups on PR #602:

1. `src/status.rs::render_blocks` synthesized fallback URL was passing
   `ado_project` verbatim into the path segment when `LastRun::url`
   was absent. Now uses `PATH_SEGMENT` like every other URL builder
   in the PR. URL is text-output only, but renders broken links for
   projects with spaces or reserved chars.

2. `src/list.rs` and `src/status.rs` were swallowing
   `detect_pipelines` errors via `.unwrap_or_default()`, making
   "detection failed" indistinguishable from "no pipelines compiled
   here" — both produce zero matches downstream. Both commands are
   read-only and useful even with partial inputs (`list --all`
   doesn't need fixtures at all), so we don't bail; we emit a
   `warning: failed to scan local pipelines: …` to stderr so the
   operator can distinguish the two cases.

3. `src/run.rs::parse_parameters` silently rejects values containing
   commas (the `,` split happens before the `=` split, so the
   trailing fragment falls into the "no `=`" rejection path). The
   behaviour is intentional — commas are the documented pair
   separator — but it was undocumented. Added a doc comment
   spelling out the constraint and the one-pair-per-flag
   workaround, plus a new
   `parse_parameters_values_with_commas_split_pre_equals` unit test
   that pins both the rejection and the workaround. The doc comment
   tells future contributors to update the test if comma escaping
   is ever added.

4. `src/secrets.rs::run_set_github_token` carries an undocumented
   invariant: the deprecation warning must be emitted before any
   fallible I/O, because the integration test
   `configure_invocation_still_works_and_warns` exercises it by
   driving the function with a path that triggers an early
   canonicalize failure. Added an `IMPORTANT — invariant for the
   integration test` doc comment so a later refactor that defers
   the `eprintln!` (e.g. lazy auth init) will spot the constraint.

`cargo test` (1568 unit + 14 integration crates, all green) and
`cargo clippy --all-targets --all-features` pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): correct parse_parameters doc; cap consecutive poll errors; warn on empty status

Three follow-ups on PR #602:

1. `parse_parameters` doc-comment bug: the first bullet was tagged
   ✅ but described the same broken case as the third (✅
   `--parameters 'urls=a,b' --parameters mode=fast` still splits on
   the comma inside `urls=a,b` and fails on the trailing `b`
   fragment). Rewrote the bullet list so all broken examples are ❌
   and only the genuine "one pair per flag, no commas in values"
   workaround is ✅. Also clarified that there is currently no way
   to escape a comma inside a single `--parameters` argument, and
   pointed at the existing
   `parse_parameters_values_with_commas_split_pre_equals` unit
   test as the behaviour anchor.

2. `poll_until_complete` couldn't distinguish a permanent error
   (deleted build, revoked PAT, 404) from a transient one — both
   pushed the target back onto `next_pending` and silently retried
   until `--timeout`. Added a per-build `consecutive_errors:
   HashMap<u64, usize>` counter that resets on any successful poll
   and bails out of that specific build after
   `MAX_CONSECUTIVE_POLL_ERRORS = 3` consecutive failures, counting
   it as failed. Transient blips still retry; persistent failures
   surface within `3 × poll_interval` (default 30s) instead of
   waiting out the full `--timeout` (default 1800s).

3. `status` was silently rendering `(no matched definitions)` when
   the fixture matcher returned zero hits, which is
   indistinguishable from running in the wrong directory. Added an
   `eprintln!` warning that mirrors the existing
   `failed to scan local pipelines: …` message. The command stays
   non-fatal (read-only) by design, unlike `disable` which bails.

`cargo test` (1568 unit + 14 integration crates, all green) and
`cargo clippy --all-targets --all-features` pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): no silent allowOverride downgrade; surface comma hint; harden dry-run

Four follow-ups on PR #602:

1. **`apply_variable_set`: silent `allowOverride` downgrade on
   secret rotation.** Previously, running `secrets set TOKEN <new>`
   without `--allow-override` would re-emit the variable with
   `allowOverride: false`, silently downgrading any variable that
   was previously created (manually or by another tool) with
   `allowOverride: true`. The legacy `configure` code in
   src/configure.rs had explicit preservation logic; the
   consolidated `apply_variable_set` had lost it.

   Changed the signature from `allow_override: bool` to
   `allow_override: Option<bool>`:
   - `Some(true)` / `Some(false)` — force the flag (CLI
     `--allow-override` passes `Some(true)`).
   - `None` — **preserve** existing variable's `allowOverride`
     when overwriting; default to `false` when creating.

   `run_set` translates the CLI flag: `--allow-override` → `Some(true)`;
   absence → `None`. The deprecation alias (`run_set_github_token`)
   stays at `allow_override: false` on the CLI side, which now maps
   to `None` (preserve) — restoring parity with the pre-consolidation
   `configure` body. Help text in `src/main.rs` and `docs/cli.md`
   updated. Five new unit tests pin the matrix:
   - `Some(true)` / `Some(false)` / `None` × create/overwrite
   - Specifically asserts `None` preserves `allowOverride: true`
     (the silent-downgrade regression guard).

2. **`run.rs::print_queue_plan` silent serialize-failure.**
   `serde_json::to_string_pretty(&body).unwrap_or_default()` would
   have printed blank output if serialization ever failed. The
   value is provably JSON-safe, but defensive code should surface
   regressions instead of silently swallowing them. Switched to
   `unwrap_or_else(|e| format!("<serialization error: {e}>"))`.

3. **`run.rs::parse_parameters` opaque comma-in-value error.**
   When a user writes `--parameters urls=https://a,b`, the error
   was `Invalid --parameters pair 'b': expected key=value (no '='
   found).` — technically accurate but doesn't hint at the comma
   constraint documented above the function. Added a
   raw-argument-contains-comma detection branch that produces a
   self-diagnosable hint: `... Hint: values must not contain
   commas. The raw argument '...' was split on ',' before the
   '=' split; use a separate --parameters flag per pair.`

4. **`run.rs::dispatch` deliberate partial-queue + `--wait`
   behaviour.** When `--wait` is set and some builds fail to
   queue, the code polls the successfully-queued ones rather than
   bailing early; `queue_failure` is folded into the final exit
   code. This is intentional and the only sensible UX, but lacked
   a comment. Added a multi-paragraph block explaining all three
   cases (partial queue, zero queued, all queued) and why
   `poll_until_complete` is called with the partial slice.

Not addressed (acknowledged follow-ups, tracked elsewhere):
- Sequential `get_latest_build` fanout in `list`/`status`. Already
  documented inline; tracked as a future improvement.

`cargo test` (1572 unit + 14 integration crates, all green) and
`cargo clippy --all-targets --all-features` pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(cli): remove bails when no fixtures found; surface --parameters comma constraint in --help

Two follow-ups on PR #602:

1. **`remove` silently exited Ok(()) when no fixtures were
   detected.** For a destructive command this is the wrong UX —
   running `ado-aw remove` in the wrong directory currently
   printed "No agentic pipelines found." and exited success,
   giving no signal that nothing happened. Now mirrors `disable`:
   bails with a non-zero exit and tells the operator which path
   was scanned plus the recovery hint:

       No local agentic pipeline fixtures were found under <path>.
       Run `ado-aw compile` first (or point `ado-aw remove` at the
       repo root). `remove` refuses to exit success in this state
       because it's destructive.

2. **`--parameters` comma constraint was documented in the module
   doc-comment but not in `--help` text.** A user who writes
   `--parameters redirect_uri=https://a,b` would only learn about
   the constraint by reading the source. Added an inline
   `VALUES MUST NOT CONTAIN COMMAS …` blurb to the clap `help`
   attribute and updated `docs/cli.md` to match. The integration
   test now asserts the constraint appears in `--help` so a
   refactor that drops the warning will be caught at CI.

`cargo test` (1572 unit + 14 integration crates, all green) and
`cargo clippy --all-targets --all-features` pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>